Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Function case #317

Merged
merged 16 commits into from
Jul 23, 2024
Merged

Function case #317

merged 16 commits into from
Jul 23, 2024

Conversation

pem70
Copy link
Collaborator

@pem70 pem70 commented Jul 18, 2024 •
edited
Loading

What It Does
Addresses [#315] by employing pylint for function name checks.

How to Test

Review Checklist
I certify that I have:

Additional Comments

@codecov Codecov
Copy link

codecov bot commented Jul 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.61538% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.90%. Comparing base (a04dde5) to head (4f44ed6).
Report is 87 commits behind head on main.

Files with missing lines Patch % Lines
src/core/zowe/core_for_zowe_sdk/profile_manager.py 41.17% 10 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #317      +/-   ##
==========================================
+ Coverage   89.89%   89.90%   +0.01%     
==========================================
  Files          53       53              
  Lines        2819     2813       -6     
==========================================
- Hits         2534     2529       -5     
+ Misses        285      284       -1
Flag Coverage Δ
unittests 89.90% <84.61%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pem70 pem70 requested review from t1m0thyj, zFernand0 and traeok July 18, 2024 17:33
@pem70 pem70 marked this pull request as ready for review July 18, 2024 17:33
@pem70 pem70 changed the base branch from main to doc-string-enforcer July 19, 2024 14:19
t1m0thyj
t1m0thyj reviewed Jul 22, 2024
View reviewed changes
.github/workflows/sdk-build.yml Outdated
Comment on lines 35 to 37
- name: Lint with pylint
run: pylint src --disable=all --enable=C0103 --ignore=build
- name: Lint with flake8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're adopting pylint then should we remove all references to flakes8 rather than having a mix of both?

  • Remove step that runs flake8 in this workflow
  • Remove pyflakes dependency from requirements.txt
  • Update .vscode/settings.json to include setting that enables Pylint in VS Code
  • Replace flakes8 with pylint in "Code Standards" section of CONTRIBUTING.md

Copy link
Member

@traeok traeok Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely think this is worth considering, but in the interest of avoiding scope creep, maybe this can be created as a separate issue?

Update: Sorry, didn't realize that pylint was not in the codebase before this PR. If there is a flake8 rule we could use, we should leverage what's already in the dependencies instead - otherwise, maybe we can create an issue to remove flake8 if there isn't another use for it.

Copy link
Member

@zFernand0 zFernand0 Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave it up to @pem70 to handle that issue as part of this PR or as part of a new one 😋

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will handle this as part of this PR

traeok
traeok approved these changes Jul 22, 2024
View reviewed changes
Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, thanks @pem70!

CHANGELOG.md Outdated Show resolved Hide resolved
@pem70 @traeok
Update CHANGELOG.md
2f96468 
Co-authored-by: Trae Yelovich <[email protected]>
Signed-off-by: Peizhao Mei <[email protected]>
@zFernand0 zFernand0 mentioned this pull request Jul 22, 2024
Closed
Base automatically changed from doc-string-enforcer to main July 23, 2024 18:29
t1m0thyj
t1m0thyj approved these changes Jul 23, 2024
View reviewed changes
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @pem70! I ran system tests and they all passed 😋

zFernand0
zFernand0 approved these changes Jul 23, 2024
View reviewed changes
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably publish a new version after this PR is merged since it contains quite a few breaking changes. 😋

That way, people have a chance to try things out before we publish a 1.0.0 (hopefully next week) 🙏🏽

@t1m0thyj t1m0thyj merged commit 307ec04 into main Jul 23, 2024
20 checks passed
@t1m0thyj t1m0thyj deleted the function_case branch July 23, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@traeok traeok traeok approved these changes

@zFernand0 zFernand0 zFernand0 approved these changes

@t1m0thyj t1m0thyj t1m0thyj approved these changes

Assignees

@pem70 pem70

Labels
None yet
Projects
Zowe CLI Squad
Status: Closed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pem70 @t1m0thyj @zFernand0 @traeok